perlipc.pod: fix the "exec from signal handler" example #22136
Conversation
Yeah I may have removed too much in that example. |
|
Yeah I may have removed too much in that example.
Thank you for having a look.
Having reread the man page (and relevant parts of POSIX and
Linux docs), I realize the paragraph I restored really talks
about an unrelated issue, which (as mentioned in
de7ba51 commit message) is unlikely to occur nowadays,
so let's keep that removed.
The POSIX::sigaction (and SA_NODEFER) is needed here only
because the signal handler calls exec: by default (with a
plain %SIG handler) the handled signal is blocked (added to
process signal mask) while the handler function runs, but
here it never returns (calling exec) and the signal mask is
preserved across execve, preventing further instances of
the signal from being delivered to the new process.
Perhaps let's also improve the code comment a bit to explain
the SA_NODEFER usage.
|
stepnem
force-pushed
the
perlipc-handler-exec
branch
from
d8adbe3 to
e676714
Compare
Maybe, or maybe it should instead use |
|
Maybe, or maybe it should instead use `sigprocmask` in the sighandler to reset the mask right before the `exec`
Yeah, although if the concern is avoiding the window for
another SIGHUP in the handler function before exec, wouldn't
it be better to unblock SIGHUP unconditionally in the main
program (outside the handler)?
(Possibly for other reasons as well: after all, the signal
might also be added to procmask by something else before
executing the program; also, the perl POSIX module has the
following caveat for sigprocmask: "Note that you can't
reliably block or unblock a signal from its own signal
handler if you're using safe signals.")
|
|
@stepnem, we have certain metadata checks that are preventing your pull request from being tested by our continuous integration rig. Could you please do the following:
Please run |
I don't think that is a concern. |
This is a partial revert of de7ba51 ("Doc patch to perlipc", 2011-09-11). I tested both the pre-de7ba5179657 and current version of the example program (section "Handling the SIGHUP Signal in Daemons") on Arch Linux (perl v5.38.2 built for x86_64-linux-thread-multi) and OpenBSD -current (perl v5.36.3 built for amd64-openbsd). On both systems, the handler is called only once with the current (broken) version of the example program. With the pre-de7ba5179657 version the handler is called on every delivery of the signal, as expected. POSIX shell test script (perlipc-sighandler.sh): ================================================ -->8-- #!/bin/sh check() { printf '\nTesting %s\n' "$*" "$@" & sleep 1 kill -HUP $! sleep 1 kill -HUP $! sleep 1 kill $! } bad=./perlipc-sighandler-bad.perl good=./perlipc-sighandler-good.perl # from current perlipc.pod (perl5 commit e78caca v5.39.9-35-ge78caca8144e) cat <<\EOF >"$bad" #!/usr/bin/perl use v5.36; use POSIX (); use FindBin (); use File::Basename (); use File::Spec::Functions qw(catfile); $| = 1; # make the daemon cross-platform, so exec always calls the script # itself with the right path, no matter how the script was invoked. my $script = File::Basename::basename($0); my $SELF = catfile( $FindBin::Bin, $script ); # POSIX unmasks the sigprocmask properly $SIG{HUP} = sub { print "got SIGHUP\n"; exec( $SELF, @argv ) || die "$0: couldn't restart: $!"; }; code(); sub code { print "PID: $$\n"; print "ARGV: @argv\n"; my $count = 0; while (1) { sleep 2; print ++$count, "\n"; } } EOF # from perlipc.pod before perl5 commit de7ba51 v5.15.2-343-gde7ba5179657 cat <<\EOF >"$good" #!/usr/bin/perl use v5.36; use POSIX (); use FindBin (); use File::Basename (); use File::Spec::Functions qw(catfile); $| = 1; # make the daemon cross-platform, so exec always calls the script # itself with the right path, no matter how the script was invoked. my $script = File::Basename::basename($0); my $SELF = catfile( $FindBin::Bin, $script ); # POSIX unmasks the sigprocmask properly my $sigset = POSIX::SigSet->new(); my $action = POSIX::SigAction->new("sigHUP_handler", $sigset, &POSIX::SA_NODEFER); POSIX::sigaction(&POSIX::SIGHUP, $action); sub sigHUP_handler { print "got SIGHUP\n"; exec( $SELF, @argv ) || die "$0: couldn't restart: $!"; } code(); sub code { print "PID: $$\n"; print "ARGV: @argv\n"; my $count = 0; while (1) { sleep 2; print ++$count, "\n"; } } EOF chmod +x "$bad" "$good" check "$bad" check "$good" -->8-- Example observed output: ======================== ; ./perlipc-sighandler.sh Testing ./perlipc-sighandler-bad.perl PID: 19120 ARGV: got SIGHUP PID: 19120 ARGV: Testing ./perlipc-sighandler-good.perl PID: 19980 ARGV: got SIGHUP PID: 19980 ARGV: got SIGHUP PID: 19980 ARGV: Cc: Leon Timmermans <[email protected]> Fixes: de7ba51 ("Doc patch to perlipc")
"POSIX unmasks the sigprocmask properly" seems rather opaque. Instead, explain the SA_NODEFER motivation directly.
Instead of using SA_NODEFER, which makes the handler
function itself interruptible by another SIGHUP, use
a plain $SIG{HUP} handler and unblock SIGHUP using
sigprocmask from the main program.
stepnem
force-pushed
the
perlipc-handler-exec
branch
from
e283461 to
75c06e2
Compare
|
@jkeenan thank you for the heads up; perhaps an update to perlhack.pod @Leont seemed unhappy about the alternative proposed in the last patch, |
I think never blocking is conceptually cleaner than having to unblock explicitly. That said, in Perl the former is a bit more complicated than the latter (even when it was failing to mark the handler as delayed/safe). Both versions are correct so it doesn't really matter much, I'm fine with merging it this way. |
|
I think never blocking is conceptually cleaner than having
to unblock explicitly.
Yeah, probably. (FWIW, IIRC none of the few Unix daemons I
checked back when preparing the patches seemed to care about
explicitly (during startup) unblocking signals they were
supposed to act on, either.)
That said, in Perl the former is a bit more complicated
than the latter (even when it was failing to mark the
handler as delayed/safe).
Thanks, this made me reread relevant parts of the POSIX
module and perlipc man pages and realize that handlers
created with POSIX::SigAction have the "safe" flag off,
making them behave differently from the default Perl>=5.8
"deferred" signal handling.
So if I understand your "failing to mark" comment correctly,
you mean that the version with SA_NODEFER is missing
something like
# Use the "deferred" signal handling scheme (default for
# regular signal handlers since Perl 5.8.0, but
# POSIX::SigAction objects are created with "safe" off).
$action->safe(1);
before the 'POSIX::sigaction(&POSIX::SIGHUP, $action);'
line?
I can add that and drop the last patch, if you think that's
better.
On that note, isn't the "try using the POSIX sigaction()
function, which bypasses Perl safe signals" advice from
perlipc not quite correct? If that were the case, there
would be no point in doing $action->safe(1). It's really
the SigAction class (producing objects with "safe" unset),
not the sigaction function, that causes the bypass, right?
|
Correct.
That would be great.
They don't really exist without each other, so the distinction is moot. |
|
> So if I understand your "failing to mark" comment correctly,
> you mean that the version with SA_NODEFER is missing
> something like
>
> ```
> # Use the "deferred" signal handling scheme (default for
> # regular signal handlers since Perl 5.8.0, but
> # POSIX::SigAction objects are created with "safe" off).
> $action->safe(1);
> ```
>
> before the 'POSIX::sigaction(&POSIX::SIGHUP, $action);' line?
Correct.
So I did that, and it doesn't work! Apparently safe signals
are incompatible with SA_NODEFER, i.e., after modifying my
test script (from the gist and the first commit message) to
mark the action "safe" as above, again only one SIGHUP is
caught, same as with the current broken version of the
example that this PR set out to fix.
> On that note, isn't the "try using the POSIX sigaction()
> function, which bypasses Perl safe signals" advice from
> perlipc not quite correct? If that were the case, there
> would be no point in doing $action->safe(1). It's really
> the SigAction class (producing objects with "safe" unset),
> not the sigaction function, that causes the bypass, right?
They don't really exist without each other, so the distinction is moot.
I really don't think the distinction is moot (you can use
sigaction() to install both safe and unsafe handlers, so
saying 'sigaction() bypasses Perl safe signals' is
incorrect: whether it does or not depends on the "safe"
attribute of the SigAction object passed to it), and given
how subtle both the behavior and its documentation are (as
witnessed by our repeated confusion here), I do think we
should try not to muddle the waters further; but that's for
a separate PR I think.
As for this one, unless you have other suggestions, it seems
we're back to "Both versions are correct so it doesn't
really matter much, I'm fine with merging it this way."? In
which case I'd leave it to your discretion whether to just
drop the last patch or not.
Thanks.
|
Just great! /s
Yeah, I think you're right.
It seems we should go for the second version. |
I was scratching my head wondering why my signal handlers
calling exec[1] didn't work, and neither did the perlipc
example.
Fortunately a search turned up
https://www.perlmonks.org/?node_id=440900 which still cites
the old (and apparently still correct) documentation.
I don't know if something changed since 2011 or if the
pertinent changes in commit de7ba51 were wrong to begin
with.
For convenience I've also put the test script demonstrating
the issue (included in the commit message, with further
details) into a gist:
https://gist.github.com/stepnem/738d7d2e21bc4ab3d1d0a715c4a8bf86
[1] Note that the exec seems important; a simpler handler
(e.g., just printing or setting a variable) appears to work
fine without POSIX.